fix(egfx): stop the desktop from tearing when the server resizes graphics output - #1977
杨成锴 (asjdf) wants to merge 7 commits into
Conversation
A Display Control resize or a Ctrl+Alt+Del left the desktop torn: blocks frozen on the previous image, regions that never refreshed again. Three separate faults, all of them cases where the client was stricter than the protocol and stricter than what Windows actually sends. ResetGraphics dropped the EGFX bitmap cache. Cache slots are not surfaces: they are connection-scoped, and MS-RDPEGFX 3.3.5.14 redefines only the output buffer, so they survive a reset. Windows depends on it, restoring the desktop almost entirely from slots filled before the reset. Dropping them made every one of those CacheToSurface blits a silent no-op. SRL decoding demanded a trailing zero byte and capped a zero run at one tile's worth of coefficients. Windows sends neither: the encoder stops emitting bits once the rest of a band is zero, and static regions produce runs far longer than the cap. Both faults discarded whole tile updates, which is what surfaced as blocks that never refresh. The stream is now read zero-padded past its end and a run is consumed one event at a time, the way FreeRDP's progressive_rfx_srl_read does. Over-reads are counted so a real desync still shows up in the logs. ResetGraphics also kept the progressive difference-tile references owned by the surfaces it implicitly destroys, so a reused surface id differenced against the old desktop. Those go; the CONTEXT and the ClearCodec glyph cache stay, because the server will not re-send them. With the cache surviving, the session framebuffer has to follow the new output before the compositor deltas from the same payload are applied — the server will not send them twice. GraphicsPipelineClient::take_reset_graphics reports the size for that, including same-size resets, since those still destroy every surface. Also implements core::error::Error for ProgressiveDecodeError and includes the inner PDU error in SessionErrorKind::Pdu, so a decode failure is diagnosable instead of collapsing to "PDU error".
The protocol-side fixes are only half of the tearing story for the web client: `ironrdp-web` never opted into MS-RDPEGFX, and its canvas never followed a size the server chose on its own. Enable the graphics pipeline (`support_dyn_vc_gfx_protocol`) and register the EGFX DVC, then keep the canvas in step with `DecodedImage`: - Sync the backing store right before the frame's `GraphicsUpdate` is drawn, not after. `ActiveStage::process` can resize `image` to follow a ResetGraphics within the same frame, so a canvas synced afterwards would show that frame at the old size. - Repaint the whole image whenever the resize actually happened. Setting `width`/`height` clears a canvas, so drawing only the frame's dirty regions would blank everything the server did not happen to repaint. - Sync after a Deactivation-Reactivation Sequence too. That replaces `image` from inside the output loop, i.e. after this frame's sync already ran, and the next event can be an idle interval away. - Make `resize` report whether the size changed, so an unchanged size stays a no-op instead of clearing and repainting every frame. `Session::desktop_size()` now tracks the size in effect rather than the one negotiated at connect, since both a reset and a reactivation change it. Resize requests deliberately leave the canvas alone: it follows the size the server actually applies, not the one that was asked for.
… harness that graded it The framebuffer resize on EGFX `ResetGraphics` was the one fault in this branch with no test behind it. Reverting it left the whole suite green, so nothing stopped a later refactor from reordering it back into a torn desktop. Cover it where it can actually run. `ironrdp-session` sets `[lib] test = false`, so its inline `#[cfg(test)]` modules never execute under `cargo test --workspace`; the test goes in `ironrdp-testsuite-core` next to the existing `composite_graphics_updates` cases. It drives a real `ActiveStage` with an open EGFX channel and feeds one payload carrying `ResetGraphics` plus the drawing that follows, with the fill placed outside the old image and inside the new one — so it only survives if the resize happened first. Reverting the fix fails it on the size assertion. Also promote the resize-stability harness this branch was graded with from a local script to `examples/rdp_stress.rs`. It talks to `ironrdp-session` directly over a real connection, drives resolution changes, and grades the decoded framebuffer on black tiles, stale tiles (the previous frame stretched over the new desktop, i.e. what tearing looks like) and seam energy on the progressive tile grid. Stale now counts toward failure alongside black: a resize that leaves the old picture behind is fully painted and perfectly non-black, so grading on blackness alone reported success on exactly the bug the harness exists to find. The settle loop and the verdict share one predicate so they cannot drift.
Clarify SRL docs and test notes, use neutral wording in egfx/web comments, and remove reference_count_for_surface that existed only for debug fields.
fddc02f to
51e9562
Compare
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
|
Sorry for off-topic here. It's surprise to see my college alumni here :) |
|
Tested this against a real GNOME Remote Desktop server, in case it helps review (and #1446). Setup: What works:
One thing still needed for grd: #2005. grd sends Tested with AI assistance; results are from automated end-to-end runs against the server. |
|
Tested this with GNOME Remote Desktop 46 (Ubuntu 24.04), which only accepts clients that use the graphics pipeline, so the web client can't connect to it without a change like this one. Setup:
Our own web-client patch for GNOME had enabled EGFX per connection and followed ResetGraphics in the same way; with this PR, we no longer need it. Merging it with #2020 only conflicts on adjacent struct fields in |
… #1977 - Sound in the web client is now Devolutions/IronRDP#2020, rebased onto master on its own, with unit tests. - Devolutions/IronRDP#1977 (another contributor's) turns the graphics pipeline on for every web connection and follows ResetGraphics: with it and our other pull requests, all GNOME and Windows suites pass without our patch 5, which goes when it lands. - The plugin sets the graphicsPipeline extension only when the IronRDP build has it, so it keeps working once that patch is gone.
|
Hi 杨成锴 (@asjdf) meanaverage (@meanaverage) It seems you have some overlapping work. Can you recommend me a review order, and what you think we should focus review effort on? |
|
Test report from Windows Server 2022 (Standard, build 20348.5622) as a multi-user desktop host, in case it helps with the review order. GNOME Remote Desktop and Windows 11 are covered above; this one isn't yet. Setup. A small headless client built on
1. First screen, 1280×1024
2. Live resize. A Display Control request after the first screen settles: 1280×1024 → 1024×768 → 1600×900. Windows answers with
On the review-order question: on Server 2022 both halves are needed. Without an SRL fix (#2010 or this PR), no graphics-pipeline session survives its first progressive upgrade pass. Without keeping the cache across We have been running an equivalent downstream patch in a browser client against Windows Server 2022 hosts since 27 September, and will drop it once this PR, or #2010 + #2011, lands. Tested with AI assistance; the numbers come from automated runs against the server. |
|
Hi Benoît Cortier (@CBenoit) — please merge this PR (#1977) as a single unit. Merge order
Session framebuffer resize ( Review effort: the product fix is small. Most of the diff is Windows 11 lab (this PR vs Same grading harness ( Command shape: cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \
--host <host> -u <user> --sizes 1300x820,1828x1004 --rounds 5 --out-dir /tmp/rdp-stress
The harness connects at the first A first smoke on the host’s then-current desktop size (so the first “resize” was a no-op) already showed the split: On the same host we also isolated session framebuffer order (the 14 lines that are now On SRL: please do not keep “strip a trailing 0x00 if present” That is the only reason not to take #2010 instead. The decoder on let Some((&terminator, payload)) = data.split_last() else {
return Err(SrlError::MissingTerminator);
};
if terminator != 0 {
return Err(SrlError::MissingTerminator);
}Two failures, both from treating the last byte as a terminator rather than payload:
MS-RDPEGFX 2.2.4.2.1.5.4 gives each component an explicit The other half of this PR's |
| // `process()` may have resized `image` to follow ResetGraphics in the same | ||
| // frame. The canvas has to match *before* the GraphicsUpdate from that frame | ||
| // is drawn. | ||
| sync_canvas_to_image( | ||
| &mut gui, | ||
| &image, | ||
| &mut draw_buffer, | ||
| self.canvas_resized_callback.as_ref(), | ||
| )?; |
There was a problem hiding this comment.
image may already have its new size here, but self.desktop_size is updated only when the queued GraphicsReset event is handled later. Should we update the Cell from image before this call so canvas_resized_callback sees the new size? The reactivation path seems to already be doing this in the right order.
There was a problem hiding this comment.
Yes — GraphicsReset is dequeued on a later iteration than the process() that already resized image, so the callback was firing against the old Cell.
Pushed in e283f3e: the Cell is now copied from image immediately before sync_canvas_to_image, same order as the reactivation path.
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match &self { | ||
| SessionErrorKind::Pdu(_) => write!(f, "PDU error"), | ||
| SessionErrorKind::Pdu(e) => write!(f, "PDU error: {e}"), |
There was a problem hiding this comment.
suggestion: Revert this, this is against the error conventions. See STYLE.md for explanations.
There was a problem hiding this comment.
Reverted. SessionErrorKind::Pdu Display is "PDU error" again; the inner error stays on source(), matching Encode/Decode and STYLE.md.
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you both for the explainer.
I’ll start by reviewing and landing this PR.
Here are some comments from me.
ResetGraphics resizes DecodedImage in the same process() as the first GraphicsUpdate, but GraphicsReset is dequeued on a later iteration. Copy the image size into the Cell first so canvas_resized_callback and desktop_size() agree. Also restore SessionErrorKind::Pdu Display to the STYLE.md convention (inner error stays on source()).
|
This pull request may overlap with #2010. Both touch crates/ironrdp-graphics/src/srl.rs and progressive.rs to decode Windows-encoded RFX Progressive SRL streams: no required trailing zero byte, zero-padded reads past the stream end, and zero runs longer than the encoder bound, with matching test renames. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
The PR's product changes (SRL zero-padding decoder, keeping the EGFX bitmap cache across ResetGraphics, clearing progressive tile references on reset, web-client EGFX enablement with canvas resize sync) are narrowly targeted and verified against the head tree; no new correctness, protocol, or safety defect was found. All published candidates are low-severity maintainability/documentation items: a spec-basis gap in the reset tile-reference comment, two pieces of test-only public API added to the released ironrdp-graphics crate (overread_bits, total_reference_count), redundant GraphicsReset event plumbing in ironrdp-web, duplicated Haven test scaffolding, and the placement of the 1,299-line rdp_stress harness in the facade crate's examples (published as an open question).
- [code-compressor] GraphicsReset event path duplicates the image-driven desktop_size/canvas sync — low 🟡 — crates/ironrdp-web/src/session.rs
Verified in head: on_reset_graphics only fires when output_size is Some, and in that same ActiveStage::process call the image is already resized from take_output_reset; the post-outputs block (session.rs ~950-965) then sets self.desktop_size from image and calls sync_canvas_to_image before the queued GraphicsReset event is ever dequeued. The entire RdpInputEvent::GraphicsReset variant, its match arm (which only converts through u16 and can miss clamped sizes the image already reflects), the EgfxHandler struct, and the input_events_tx field threaded through ConnectParams/connect can be deleted — keeping at most a debug! in a no-op handler — with behavior preserved, removing one enum variant, one channel message type, and two struct fields of redundant plumbing. - [code-compressor] Two new Haven tests duplicate the CONTEXT-only init and decode-expect-failure flow — low 🟡 — crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs
haven_wts2_cold_decode_reproduces_missing_tile_reference and haven_wts2_mixed_25tiles_cold_decode_hits_missing_tile_reference each re-encode the identical ~25-line Sync/Context/FrameBegin/Region/FrameEnd init stream and repeat the same decode/match/panic/expect-failure shape; only the fixture, decode dimensions, and assertion precision differ. Extracting a context_only_init() helper and a cold_decode_error(decoder, pdu, w, h) helper (or a second #[case] on the existing rstest with an any-MissingTileReference expectation) collapses the second test to a few lines and removes drift risk as the init stream evolves. Behavior-preserving, test-only.
| // Tile coefficient buffers belong to those surfaces. Keeping them lets a | ||
| // later surface reuse the same id and difference against the previous | ||
| // desktop, which decodes as torn or duplicated tiles. | ||
| // CONTEXT / ClearCodec glyph cache stay: 3.3.5.14 only redefines the | ||
| // output buffer, and Windows will not re-send SYNC + CONTEXT. | ||
| self.progressive_decoder.clear_tile_references(); |
There was a problem hiding this comment.
[protocol] Tile references dropped on ResetGraphics rests on a surface-destruction rule the cited sections do not contain — low 🟡 — The new clear_tile_references call cites MS-RDPEGFX 2.2.2.14 and 3.3.5.14 as requiring that ResetGraphics destroys every surface, but in the corpus 3.3.5.14 only mandates resizing the graphics output buffer and 2.2.2.14 describes PDU fields; 3.3.1.3 instead requires sub-band diffing tile contexts to be preserved for the connection or until the associated surface is deleted. A spec-strict server that retains surfaces and reuses a surface id with difference tiles now gets MissingTileReference instead of preserved state. The behavior is safer than the previous silent cross-reset differencing and matches observed Windows, but the normative justification is absent and the protocol-visible error path for conformant-but-different servers is undocumented; the comment should be corrected or the caveat noted.
| /// Bits read past the end of the stream, i.e. how much of the tail was assumed to be zero. | ||
| /// | ||
| /// A handful at the very end is normal (the encoder stops once the rest of a band is zero). | ||
| /// A large count means the decoder and the encoder disagree about the stream layout. | ||
| pub fn overread_bits(&self) -> u32 { | ||
| self.reader.overread_bits | ||
| } |
There was a problem hiding this comment.
[skeptical + code-compressor] overread_bits instrumentation has no production reader despite its stated purpose — low 🟡 — The PR removes the only malformed-stream signals (MissingTerminator, Truncated) and makes the decoder silently zero-pad past the stream end; the replacement diagnostic overread_bits() is documented as keeping desyncs 'visible in the logs', but a repository search shows no caller outside its own unit test (counts_bits_read_past_the_end) — neither the progressive decode path nor any client logs the counter. As merged it is write-only state plus permanent public API surface on the released ironrdp-graphics crate. Either wire the count into a debug/warn at the decode call site or drop the field and accessor.
| /// Total retained difference-tile coefficient buffers across all surfaces. | ||
| #[must_use] | ||
| pub fn total_reference_count(&self) -> usize { | ||
| self.references.len() | ||
| } |
There was a problem hiding this comment.
[skeptical + code-compressor] Public total_reference_count added to a released crate solely for one cross-crate test — low 🟡 — total_reference_count is a #[must_use] pub method on the production ProgressiveDecoder whose only callers are the two assertions in ironrdp-egfx's reset_graphics_drops_tile_references_of_implicitly_destroyed_surfaces test. The decoder-level guarantee can be asserted inside ironrdp-graphics's own tests (which see the private references field), and the egfx test can assert the observable outcome instead — a following difference tile failing with MissingTileReference, the failure class its doc comment already cites. As merged it adds permanent public API surface for test convenience.
| //! Standalone RDP resize-stability stress harness. | ||
| //! | ||
| //! Connects straight to an RDP server over TCP + TLS/CredSSP, negotiates EGFX and | ||
| //! Display Control, then drives resolution changes and key injection while grading | ||
| //! the decoded framebuffer. No browser and no WASM: everything here talks to | ||
| //! `ironrdp-session` directly, so a failure points at the protocol/decode path | ||
| //! rather than at a client's canvas plumbing. | ||
| //! | ||
| //! Three metrics, all computed locally so they cannot be fooled by the code under test: | ||
| //! | ||
| //! - black tiles: how much of the picture is missing outright. | ||
| //! - stale tiles: how much of the picture is still the previous frame stretched over the | ||
| //! new desktop size, i.e. content the server never repainted after ResetGraphics. | ||
| //! This is what "torn"/"ghosted" looks like on screen. | ||
| //! - seam score: edge energy on the 64-pixel RemoteFX tile grid relative to tile | ||
| //! interiors. Mismatched tiles show up as a hard grid. | ||
| //! | ||
| //! # Usage example | ||
| //! | ||
| //! ```shell | ||
| //! cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \ | ||
| //! --host rdp.example.com -u Administrator --rounds 10 --out-dir /tmp/rdp-stress | ||
| //! ``` | ||
| //! | ||
| //! The password is read from `--password` or, preferably, the `RDP_PASSWORD` env var. | ||
|
|
||
| #![allow(unused_crate_dependencies)] // false positives because there is both a library and a binary | ||
| #![allow(clippy::print_stdout)] | ||
| // The grading code is percentage arithmetic over tile and pixel counts: every value is a | ||
| // small count or a 0..=100 ratio, so f32 has room to spare and a lost fraction of a | ||
| // percent cannot change a verdict. | ||
| #![allow( | ||
| clippy::as_conversions, | ||
| clippy::cast_precision_loss, | ||
| clippy::cast_possible_truncation | ||
| )] | ||
|
|
||
| use core::sync::atomic::{AtomicU32, Ordering}; | ||
| use core::time::Duration; | ||
| use std::io::Write as _; |
There was a problem hiding this comment.
[skeptical] 1,300-line interactive stress harness added to the facade crate's examples — low 🟡 ❓ — The example bundles a complete RDP client (CredSSP via sspi, TLS, keyboard injection, RDP_PASSWORD handling) plus a frame-grading engine into crates/ironrdp/examples, whose existing examples are small demos, while the repository keeps interactive tooling in dedicated crates (ironrdp-replay-client, ironrdp-capture-replay, ironrdp-bench). It is feature-gated with doc-scrape-examples=false and the PR body justifies it as the grading harness that produced the Windows 11 evidence, so this is a placement and long-term maintenance-surface question rather than a correctness defect; whether maintainers want a standalone tooling crate cannot be settled from the repository alone.
There was a problem hiding this comment.
Yeah, could be a dedicated crate at this point. 杨成锴 (@asjdf) do you think that could be a useful tool in general?
There was a problem hiding this comment.
Agreed — it is useful beyond this PR as a live graphics-pipeline / Display Control grader (the class of bug unit tests cannot see), but it is not a small library example.
Moved in 77d05a6 to an unpublished ironrdp-stress crate, next to ironrdp-capture-replay / ironrdp-bench. Run with:
cargo run -p ironrdp-stress -- --host <host> -u <user> --rounds 10 --out-dir /tmp/rdp-stressIt still needs a real host, so it stays out of cargo test.
There was a problem hiding this comment.
I would not call it a helper tool. It is a live integration test for the graphics pipeline: it drives Display Control against a real host and scores the framebuffer after ResetGraphics. That needs a real machine, so it stays an unpublished crate and out of cargo test.
|
Thanks Benoît Cortier (@CBenoit), and sorry for the slow reply. I see you've started on #1977, and that's the order I'd suggest too. My PRs don't touch its files, except #2020 (below). On the SRL question 杨成锴 (@asjdf) raised: we fixed the same Windows 11 failure in our own build before #1977 came up, and ended up with the same decoding. The last byte is data, and zero-run code words are read one at a time, as values need them. So we'd take #1977's version over stripping a trailing zero. After #1977, in this order:
#2018 is merged, thanks. Please leave #2026 out of the queue for now; I'll answer your design questions on that PR first. Where to spend review effort: #2020. It's the only one that adds public API: an |
examples/ is for small library demos. This is a live-host analysis tool in the same class as ironrdp-capture-replay, so it belongs in its own unpublished crate rather than the facade examples.
Fixes screen tearing / ghosting after the server changes the graphics output size.
Rebased onto current
master, which already resizes the session framebuffer onResetGraphicsviatake_output_resetandreset_preserving_pointerbefore compositor deltas are applied. This PR does not changeactive_stage.rs. It fixes the EGFX/graphics decode path andironrdp-webso the pixels written into that buffer are correct.The bug
When the server resizes the graphics output (EGFX
ResetGraphics, or a Deactivation-Reactivation Sequence), stale pixels survive the transition. What you see is part of the old desktop still on screen, or blocks that never repaint until something happens to overdraw them.Several independent faults contribute; any one of them can produce the symptom alone:
ResetGraphicsdropped the wrong caches. The bitmap cache was cleared, but MS-RDPEGFX 3.3.5.14 only redefines the output buffer — cache slots are connection-scoped. Windows restores the desktop after a resolution change almost entirely from slots filled before the reset, so dropping them makes hundreds ofCacheToSurfaceblits silent no-ops.Srl(MissingTerminator)on real Windows hosts.master). Deltas for the new geometry must land in a buffer already sized for the new output. Upstream now does this withtake_output_reset+reset_preserving_pointerbeforecomposite_graphics_updates. This PR assumes that base; it does not reimplement it.On top of that, the web client never opted into MS-RDPEGFX, and its canvas never followed a size the server picked on its own — so even with the protocol side correct,
ironrdp-webstill tore.Where to look first
Most of the diff is not the fix.
ironrdp-stressis a 1299-line live grading harness with no product code in it,ironrdp-testsuite-coreadds integration tests, and roughly a third of the remaining product diff is comment recording spec reasoning. The behavioural changes in this PR are small; the table is everything that still differs frommaster.compositor.rs::resetallocated_bytesto surviving slotsCacheToSurfacefrom slots filled before the reset. Clearing them turns hundreds of those blits into silent no-ops, so the old pixels stay where they were.client.rs::handle_reset_graphicsandprogressive.rs::clear_tile_referencessrl.rsSrl(MissingTerminator)), and static tiles never refresh when streams end early.ironrdp-webEGFX enablesupport_dyn_vc_gfx_protocol: trueand register the EGFX DVCsync_canvas_to_imageDecodedImagebefore drawing; full repaint when the backing store size changesActiveStage::processcan resize the image in the same frame; assigning canvaswidth/heightclears it. Dirty-rectangle painting alone would leave the rest blank.Rows 1–3 are protocol/decode faults; rows 4–5 are web embedding. Each of 1–3 can produce visible tearing or a dead session on its own.
What changed
Protocol (
ironrdp-egfx,ironrdp-graphics) — this diffResetGraphicsretains the bitmap cache, keeps the progressive CONTEXT and the ClearCodec glyph cache, and clears progressive tile references.MissingTileReference.SessionErrorKind::Pducarries the inner error;ProgressiveDecodeErrorimplementscore::error::Error.Session (
ironrdp-session) — upstream base, not modified heretake_output_reset→reset_preserving_pointer→composite_graphics_updates, plus output-dimension limits viaCompositor::materializable_output_size.Web (
ironrdp-web) — this diffDecodedImagebefore the frame'sGraphicsUpdateis drawn.Canvas::resizereports whether the size changed;Session::desktop_size()tracks the size in effect.Resize requests deliberately do not touch the canvas: it follows the size the server actually applies. Notably
SuppressOutput/RefreshRectare not sent on reset — with RDPGFX those PDUs do not invalidate the surface cache (FreeRDP#12723).cargo run -p ironrdp-stress -- --redrawexists to re-check that against a real server.Testing
compositor::tests::reset_keeps_the_bitmap_cacheclient::tests::reset_graphics_drops_tile_references_of_implicitly_destroyed_surfacessession::active_stage::egfx_reset_resizes_the_image_before_compositing_the_same_payload(fullActiveStage+ EGFX; fill outside the pre-reset image, inside the new one)session::active_stage::reset_graphics_preserves_software_pointer_state(upstream; unchanged by this PR)progressive::tests::upgrade_pass_completes_when_the_srl_stream_ends_early,tile_upgrade_keeps_all_components_on_srl_errorIntegration tests live in
ironrdp-testsuite-corebecauseironrdp-sessionsets[lib] test = false.Grading a real session
ironrdp-stressdrives resolution changes against a live server and grades the decoded framebuffer (black / stale / seam metrics). Black and stale count toward failure so a fully painted but ghosted resize does not pass.cargo run -p ironrdp-stress -- \ --host <host> -u <user> --rounds 10 --out-dir /tmp/rdp-stressOn a Windows 11 host with RDPGFX, cycling
1300x820↔1828x1004, 5 rounds:master)stale_tiles=0.00%; frame size matches the requested dimensionsmasterwithout this PRSrl(MissingTerminator)(fault 3), so resize grading never runsThe harness connects at the first
--sizesentry, so round 1's opening step can show high stale % on an idle desktop (no server repaint) — a metric artefact, not leftover tearing.